obsservice: initial commit #81598
obsservice: initial commit #81598
Conversation
|
This change is |
There was a problem hiding this comment.
Looking good - just a curiosity question & docs nit from me.
Should we include some kind of README in the pkg/obsservice?
Is it too early to consult security on the TLS piece of this?
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @andreimatei and @dhartunian)
pkg/obsservice/lib/lib.go line 140 at r1 (raw file):
// Separate HTTP traffic from HTTPS traffic. protocolMux := cmux.New(listener) clearL := protocolMux.Match(cmux.HTTP1())
nit: can we add a small comment here noting that the order in which the matchers are added to the mux dictates the priority of the matchers?
Code quote:
clearL := protocolMux.Match(cmux.HTTP1())
tlsL := protocolMux.Match(cmux.Any())pkg/obsservice/lib/lib.go line 244 at r1 (raw file):
// nil, the system defaults are used. // // HTTPTohTTPSErr, if set, will cause the proxy to detect when CRDB performs a
nit: fix capitalization
Code quote:
HTTPTohTTPSErrpkg/obsservice/lib/lib.go line 256 at r1 (raw file):
req.URL.Scheme = target.Scheme req.URL.Host = target.Host targetQuery := target.RawQuery
Out of curiosity, what would be an example of a case where the target URL as defined by crdb-http-url contains query parameters?
There was a problem hiding this comment.
Should we include some kind of README in the pkg/obsservice?
Yes. In fact I had one but the cat seems to have eaten it. I've put (the skeleton of) one now.
Is it too early to consult security on the TLS piece of this?
I think "it's never too early" is the textbook answer here. @catj-cockroach would you be kind enough to take a look and perhaps opine on this? You've been suggested as a good victim. The context is that this is the first timid piece of the Observability Service that you might have heard of. The project as a whole is described in https://docs.google.com/document/d/1DGcdBNVOWYegrbFOXOVhUaWuGvERJCuVinThmXm6LUM/edit. What's here for now is simply a new binary that acts as a reverse HTTP proxy for a CRDB cluster: it takes in a CRDB http URL, and forwards all requests to it. In time, this binary will take on more and more HTTP routes and handle them itself, while continuing to forward what it doesn't handle to CRBD. The proxy deals with things like http-to-https redirection, and it can be configured with TLS certs. If you're interested, please take a look at the certificate flags and see if things make sense to you. I'm happy to talk about it if anything is unclear or doesn't make sense. Thanks!
Reviewable status:
pkg/obsservice/lib/lib.go line 140 at r1 (raw file):
Previously, abarganier (Alex Barganier) wrote…
nit: can we add a small comment here noting that the order in which the matchers are added to the mux dictates the priority of the matchers?
done
pkg/obsservice/lib/lib.go line 244 at r1 (raw file):
Previously, abarganier (Alex Barganier) wrote…
nit: fix capitalization
done
pkg/obsservice/lib/lib.go line 256 at r1 (raw file):
Previously, abarganier (Alex Barganier) wrote…
Out of curiosity, what would be an example of a case where the target URL as defined by
crdb-http-urlcontains query parameters?
The only thing that I had in mind was the parameter for directing your requests to a specific node - like if the URL is a balancer, but you want CRDB to route to a specific node. I've adapted this code from the stdlib; I have already removed some massaging from it that seemed overkill, but this one I thought doesn't hurt. Would you rather I get rid of it?
There was a problem hiding this comment.
Reviewed 3 of 6 files at r1, 5 of 8 files at r2, all commit messages.
Reviewable status:
pkg/testutils/lint/lint_test.go line 250 at r2 (raw file):
} } else { if bslHeader.Find(data) == nil && apacheHeader.Find(data) == nil {
nit: consider isApache like above that filters on obsservice, otherwise copy pasting might get us in trouble.
There was a problem hiding this comment.
Reviewable status:
pkg/testutils/lint/lint_test.go line 250 at r2 (raw file):
Previously, dhartunian (David Hartunian) wrote…
nit: consider
isApachelike above that filters onobsservice, otherwise copy pasting might get us in trouble.
done
These are the humble beginnings of an Observability Service for CRDB. In time, this is meant to take over a lot of the observability functionality, extracting it from the database itself. For now, all there is here is an HTTP reverse proxy, able to route HTTP requests to a CRDB node (or cluster, through a load balancer). At least for the transitioning period, if not forever, the Obs Service will route HTTP paths that it doesn't understand to the Cockroach cluster being monitored. The HTTP proxy accepts its own certificates and can serve HTTPS independently of CRDB (which might be running in --insecure mode and not serving HTTPS). However, if CRDB is serving HTTPS, then the Obs Service must also be configured with a certificate so it can also serve HTTPS. The code of the Obs Service is split in between a binary (minimal shell), and a library. The idea is to keep the door open for other repos to extend the Obs Service with extra functionality (e.g. the CC managed service might want to add cloud-specific observability features). For such purposes, I've considered making a dedicated Go module for the Obs Service so that source-level imports of the Obs Service would be as ergonomic as Go wants them to be. I've put a decent amount of work in playing with this but, ultimately, I've decided against it, at least for now. The problem is that the Obs Service wants to take minor code dependencies on random CRDB libraries (syncutil, log, etc.) and the cockroach Go module is fairly broken (our git tags look like we do semantic versioning, but we don't actually do it). The go tooling has a terrible time with the cockroach module. Also, our code is generally not `go build`-able. If the obs service was a dedicated module, it would need to take a dependency on the cockroach module, which would negate the win for people who want to import it (in fact, it'd make matters worse for importers because the nasty cockroach dependency would be more hidden). An alternative I've considered was to start creating modules for all of the dependencies of the Obs Service. But, if CRDB would then need to depend on these modules, there's all sorts of problems. Release note: None
There was a problem hiding this comment.
Reviewable status:
pkg/obsservice/lib/lib.go line 256 at r1 (raw file):
Previously, andreimatei (Andrei Matei) wrote…
The only thing that I had in mind was the parameter for directing your requests to a specific node - like if the URL is a balancer, but you want CRDB to route to a specific node. I've adapted this code from the stdlib; I have already removed some massaging from it that seemed overkill, but this one I thought doesn't hurt. Would you rather I get rid of it?
Thanks for the explanation (and apologies for the late response) - Nah I think the way it is LGTM, I was mostly just curious.
There was a problem hiding this comment.
TFTRs!
bors r+
Reviewable status:
|
Build succeeded: |
These are the humble beginnings of an Observability Service for CRDB. In
time, this is meant to take over a lot of the observability
functionality, extracting it from the database itself. For now, all
there is here is an HTTP reverse proxy, able to route HTTP requests to a
CRDB node (or cluster, through a load balancer). At least for the
transitioning period, if not forever, the Obs Service will route HTTP
paths that it doesn't understand to the Cockroach cluster being
monitored.
The HTTP proxy accepts its own certificates and can serve HTTPS
independently of CRDB (which might be running in --insecure mode and not
serving HTTPS). However, if CRDB is serving HTTPS, then the Obs Service
must also be configured with a certificate so it can also serve HTTPS.
The code of the Obs Service is split in between a binary (minimal
shell), and a library. The idea is to keep the door open for other repos
to extend the Obs Service with extra functionality (e.g. the CC managed
service might want to add cloud-specific observability features). For
such purposes, I've considered making a dedicated Go module for the Obs
Service so that source-level imports of the Obs Service would be as
ergonomic as Go wants them to be. I've put a decent amount of work in
playing with this but, ultimately, I've decided against it, at least for
now. The problem is that the Obs Service wants to take minor code
dependencies on random CRDB libraries (syncutil, log, etc.) and the
cockroach Go module is fairly broken (our git tags look like we do
semantic versioning, but we don't actually do it). The go tooling has a
terrible time with the cockroach module. Also, our code is generally not
go build-able. If the obs service was a dedicated module, it wouldneed to take a dependency on the cockroach module, which would negate
the win for people who want to import it (in fact, it'd make matters
worse for importers because the nasty cockroach dependency would be more
hidden). An alternative I've considered was to start creating modules
for all of the dependencies of the Obs Service. But, if CRDB would then
need to depend on these modules, there's all sorts of problems.
Release note: None